Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Fail early on StateDB functions #566

Merged
merged 11 commits into from
Sep 17, 2021
Merged

Fail early on StateDB functions #566

merged 11 commits into from
Sep 17, 2021

Conversation

davcrypto
Copy link
Contributor

@davcrypto davcrypto commented Sep 16, 2021 •
edited
Loading

Closes: #410

Description

Introduce stateErr fields into keeper so that stateDB functions enabled ability to detect previous error

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@orijbot
Copy link

orijbot commented Sep 16, 2021

Visit https://dashboard.github.orijtech.com?back=0&pr=566&remote=true&repo=davcrypto%2Fethermint to see benchmark details.

@codecov
Copy link

codecov bot commented Sep 16, 2021 •
edited
Loading

Codecov Report

Merging #566 (205d167) into main (0463c8b) will decrease coverage by 0.67%.
The diff coverage is 19.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
- Coverage   53.39%   52.72%   -0.68%     
==========================================
  Files          64       64              
  Lines        5128     5233     +105     
==========================================
+ Hits         2738     2759      +21     
- Misses       2263     2320      +57     
- Partials      127      154      +27
Impacted Files Coverage Δ
x/evm/keeper/statedb.go 73.35% <14.00%> (-12.87%) ⬇️
x/evm/keeper/keeper.go 74.56% <100.00%> (+0.14%) ⬆️
x/evm/keeper/state_transition.go 74.90% <100.00%> (+0.59%) ⬆️

fedekunze
fedekunze suggested changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good in general, but the error is not cleared anywhere (I only see it on the test).

Can you also add a changelog entry under Improvements? 🙏

x/evm/keeper/statedb.go Outdated Show resolved Hide resolved
x/evm/keeper/statedb.go Show resolved Hide resolved
x/evm/keeper/statedb.go Outdated Show resolved Hide resolved
x/evm/keeper/statedb.go Show resolved Hide resolved
@davcrypto @fedekunze
Update x/evm/keeper/statedb.go
fa2dcde 
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@davcrypto
Copy link
Contributor Author

Thanks @fedekunze for the review. Made some update and explanation according to the your comments. Lets discuss on it

fedekunze
fedekunze reviewed Sep 16, 2021
View reviewed changes
x/evm/keeper/statedb.go Outdated Show resolved Hide resolved
@fedekunze fedekunze enabled auto-merge (squash) September 17, 2021 06:13
@fedekunze fedekunze added the automerge Automatically merge PR once all prerequisites pass. label Sep 17, 2021
@fedekunze fedekunze merged commit 742b6d1 into evmos:main Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedekunze fedekunze fedekunze approved these changes

@khoslaventures khoslaventures Awaiting requested review from khoslaventures

Assignees
No one assigned
Labels
automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fail early on StateDB functions
3 participants
@davcrypto @orijbot @fedekunze